Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(test): load to stream tmp file counting #13366

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented Jul 19, 2024

Replaces #13037

Motivation

This test has an infrequent flake, see #13037

Modifications

The current test counts files in /tmp and hopes there are the same number before and after the test, which is fragile

Add a prefix to the stream temporary files.
Only count files in /tmp which have this prefix.

Verification

Tested locally. Tested with a file called /tmp/wfstream-asdfasd and extra logging in the test to show the numbers are as expected.
Tested with the os.Remove() removed from selfDestructingFile.Close() to show failure.

@Joibel Joibel marked this pull request as ready for review July 19, 2024 09:24
@agilgur5 agilgur5 added type/tech-debt area/artifacts S3/GCP/OSS/Git/HDFS etc area/build Build or GithubAction/CI issues labels Jul 19, 2024
Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for root causing this!

workflow/artifacts/common/load_to_stream.go Outdated Show resolved Hide resolved
The current test counts files in /tmp and hopes there are the same
number before and after the test, which is fragile

Add a prefix to the stream temporary files.

Only count files in /tmp which have this prefix.

Signed-off-by: Alan Clucas <[email protected]>
@Joibel Joibel force-pushed the fix-test-load-to-stream-fs-race branch from 3258dff to 7f545ac Compare July 25, 2024 13:48
Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, replacement with os.CreateTemp deferred as it requires a more substantial refactor

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 26, 2024
@agilgur5 agilgur5 merged commit 637ffce into main Jul 26, 2024
28 checks passed
@agilgur5 agilgur5 deleted the fix-test-load-to-stream-fs-race branch July 26, 2024 13:53
Joibel added a commit to pipekit/argo-workflows that referenced this pull request Sep 19, 2024
Joibel added a commit that referenced this pull request Sep 20, 2024
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/artifacts S3/GCP/OSS/Git/HDFS etc area/build Build or GithubAction/CI issues lgtm This PR has been approved by a maintainer type/tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants